-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the device.app.lifecycle event description and constraints #673
Conversation
82cb599
to
26da435
Compare
26da435
to
a8ef157
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think everything is looking good for Android, I added some comments mostly to clarify some things and to get opinions on the state
attr name. But I'm fine to go with these changes as they are. I just would like to wait for @bryce-b's opinion on the iOS part.
|
||
<!-- semconv ios.lifecycle.events --> | ||
<!-- semconv device.app.lifecycle(full) --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious what does the (full)
part means?
example, if the `os.name` attribute is set to `android` then the | ||
`android.state` attribute MUST be used and the `ios.state` attribute MUST NOT | ||
be used. If the `os.name` attribute is set to `ios` then the `ios.state` | ||
attribute MUST be used and the `android.state` attribute MUST NOT be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is the way it was before, although after reading this paragraph, it seems to me that it might not be necessary to prepend the OS name (either android.
or ios.
) to the state
attr name, given that the os.name
attr must be present anyway, rendering the OS prefixes in state
redundant in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the desire to have the prefix due to the fact that we want to allow a separate list of valid values for each?
|
||
The `android.state` and `ios.state` attributes are mutually exclusive and MUST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some native mobile apps where there are two platforms at play. For instance, Unity apps are written in a cross-platform way and can be deployed on both iOS and Android, in which case we want to convey that it's a Unity app and the event is firing in a specific platform.
If we have this mutual exclusivity, how can we represent that? I can see at least two options:
- Allow duplication of states, and rely on the fact that an event can't both be iOS and Android at the same time, but could potentially be one of each, and then also be Unity, React Native, Kotlin Multiplatform, etc.
- Have a separate (optional) attribute that represents the framework used to generate the app, and keep the actual OS state mutually exclusive.
BTW, this may be beyond the scope of this change - if so, I can submit another pull request to add this.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
Reformat and update the
device.app.lifecycle
event description addsconstraints for the possible values of the
android.state
andios.state
.Merge requirement checklist